Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix wchar deserialize error when swaping the bytes #186

Merged
merged 3 commits into from
Feb 21, 2024
Merged

Fix wchar deserialize error when swaping the bytes #186

merged 3 commits into from
Feb 21, 2024

Conversation

hwwei521
Copy link
Contributor

After downloading the latest Fast-CDR, I verified Fast-CDR's functionality by following the steps below：

git clone https://github.com/eProsima/Fast-CDR.git

cd Fast-CDR

mkdir build && cd build

cmake .. -DCMAKE_INSTALL_PREFIX=../install -D BUILD_TESTING=ON -D GTest_DIR=/usr/local

cmake --build . --target install

./test/cdr/UnitTests

When I wanted to test the performance in the case of CDR swap, I changed the Cdr cdr_ser(cdrbuffer) and Cdr cdr_des(cdrbuffer) of the TEST(CDRTests, Complete) test entry to Cdr cdr_ser(cdrbuffer, eprosima::fastcdr::Cdr::Endianness::BIG_ENDIANNESS) and Cdr cdr_des(cdrbuffer, eprosima::fastcdr::Cdr::Endianness::BIG_ENDIANNESS) respectively. To make the test and the current test environment default processor Endianness inconsistent, forced to trigger the processing of CDR swap, found in EXPECT_EQ (wcscmp(c_wstring_t, c_wstring_value), 0) reported an error.

I found that swap is not considered in the Cdr& Cdr::serialize(wchar_t*& string_t) function, so I replaced it with deserialize_array(string_t, length), and it works fine.

hwwei521
hwwei521 commented Dec 29, 2023
View reviewed changes
Copy link
Contributor Author

@hwwei521 hwwei521 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems work fine

@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

hwwei521
hwwei521 commented Dec 29, 2023
View reviewed changes
Copy link
Contributor Author

@hwwei521 hwwei521 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richiprosima Hi richiprosima,
Can you help review the code and allow patch submissions?
This patch will fix issues #185.

@JLBuenoLopez
Copy link
Contributor

@hwwei521

Please be patient. Currently most of eProsima developers are in holiday vacations. We will review your contribution in the next few weeks. Sorry for the wait.

@JesusPoderoso
Copy link
Contributor

Hi @hwwei521 thank you for using Fast DDS and for your contribution!

When I wanted to test the performance in the case of CDR swap, I changed the Cdr cdr_ser(cdrbuffer) and Cdr cdr_des(cdrbuffer) of the TEST(CDRTests, Complete) test entry to Cdr cdr_ser(cdrbuffer, eprosima::fastcdr::Cdr::Endianness::BIG_ENDIANNESS) and Cdr cdr_des(cdrbuffer, eprosima::fastcdr::Cdr::Endianness::BIG_ENDIANNESS) respectively. To make the test and the current test environment default processor Endianness inconsistent, forced to trigger the processing of CDR swap, found in EXPECT_EQ (wcscmp(c_wstring_t, c_wstring_value), 0) reported an error.

Could you include another commit with a regression test including the way you test it (as you have described above) to keep track of it?

@hwwei521
Copy link
Contributor Author

Hi @hwwei521 thank you for using Fast DDS and for your contribution!

When I wanted to test the performance in the case of CDR swap, I changed the Cdr cdr_ser(cdrbuffer) and Cdr cdr_des(cdrbuffer) of the TEST(CDRTests, Complete) test entry to Cdr cdr_ser(cdrbuffer, eprosima::fastcdr::Cdr::Endianness::BIG_ENDIANNESS) and Cdr cdr_des(cdrbuffer, eprosima::fastcdr::Cdr::Endianness::BIG_ENDIANNESS) respectively. To make the test and the current test environment default processor Endianness inconsistent, forced to trigger the processing of CDR swap, found in EXPECT_EQ (wcscmp(c_wstring_t, c_wstring_value), 0) reported an error.

Could you include another commit with a regression test including the way you test it (as you have described above) to keep track of it?

Hi, Jesús
You want me to submit a patch of the program I was testing at the time to do a regression test right?
I was doing the test as below, and if I need to submit a patch for regression testing, I'll submit another patch later for regression test validation use.

git clone https://github.com/eProsima/Fast-CDR.git

cd Fast-CDR

mkdir build && cd build

cmake .. -DCMAKE_INSTALL_PREFIX=../install -D BUILD_TESTING=ON -D GTest_DIR=/usr/local

cmake --build . --target install

./test/cdr/UnitTests

@JesusPoderoso
Copy link
Contributor

Hi Hanwuwei, I meant including a test like this one in the test suite, where we can try to run with and without your fix to check that is solves it.

@hwwei521
Copy link
Contributor Author

hwwei521 commented Jan 15, 2024 •
edited
Loading

Hi Hanwuwei, I meant including a test like this one in the test suite, where we can try to run with and without your fix to check that is solves it.

Hi, Jesús
I update another patch #189, Please help to check.
#189
The test result as the file show
UnitTests_reports.txt

@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@hwwei521
Copy link
Contributor Author

@JesusPoderoso
Hi, Jesús, The simple regression test patch added in #190

@EduPonz EduPonz added this to the v2.2.0 milestone Feb 15, 2024
@MiguelCompany MiguelCompany mentioned this pull request Feb 19, 2024
Closed
weihanwu and others added 2 commits February 20, 2024 12:50
@MiguelCompany
Regression test for eProsima#185 in SimpleTest
1559a6b 
Signed-off-by: weihanwu <weihanwu@lixiang.com>
@MiguelCompany
Fix wchar deserialize error when swaping the bytes
d6ee7d2 
Signed-off-by: weihanwu <weihanwu@lixiang.com>
Co-authored-by: maxin <maxin@lixiang.com>
@MiguelCompany
Copy link
Member

I rebased this on top of master, and included the test from #190

@MiguelCompany MiguelCompany mentioned this pull request Feb 20, 2024
Closed
@MiguelCompany MiguelCompany linked an issue Feb 20, 2024 that may be closed by this pull request
wchar deserialize error when swaping the bytes #185
Closed
richiware
richiware requested changes Feb 20, 2024
View reviewed changes
test/cdr/SimpleTest.cpp Outdated Show resolved Hide resolved
@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@MiguelCompany @richiware
Apply suggestion
5a0eb23 
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Co-authored-by: Ricardo González <correoricky@gmail.com>
@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@MiguelCompany
Copy link
Member

Going in. Thank you @hwwei521 for your contribution!

@MiguelCompany MiguelCompany merged commit 3b94bdb into eProsima:master Feb 21, 2024
8 of 12 checks passed
EduPonz pushed a commit that referenced this pull request Feb 22, 2024
@hwwei521 @MiguelCompany
@richiware @EduPonz
Fix wchar deserialize error when swaping the bytes (#186)
adfd1ce 
* Regression test for #185 in SimpleTest

Signed-off-by: weihanwu <weihanwu@lixiang.com>

* Fix wchar deserialize error when swaping the bytes

Signed-off-by: weihanwu <weihanwu@lixiang.com>
Co-authored-by: maxin <maxin@lixiang.com>

* Apply suggestion

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Co-authored-by: Ricardo González <correoricky@gmail.com>

---------

Signed-off-by: weihanwu <weihanwu@lixiang.com>
Co-authored-by: maxin <maxin@lixiang.com>
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
Co-authored-by: Ricardo González <correoricky@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richiware richiware richiware approved these changes

Assignees
No one assigned
Labels
ci-pending
Projects
None yet
Milestone
v2.2.0
Development

Successfully merging this pull request may close these issues.

wchar deserialize error when swaping the bytes
7 participants
@hwwei521 @richiprosima @JLBuenoLopez @JesusPoderoso @MiguelCompany @richiware @EduPonz